Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration files may now also be stored under sys.prefix #6268

Merged
merged 8 commits into from
Mar 7, 2019

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Feb 15, 2019

Adds sys.prefix as a valid location for global configuration.

Fixes #5060

@zooba
Copy link
Contributor Author

zooba commented Feb 15, 2019

This is the most trivial approach, and figured it was enough to start a concrete discussion rather than the abstract one over on the bug.

My main concern is that I think this approach makes it the config file that will be modified by pip config set --global …? And I'm not sure that's what we want...

@zooba
Copy link
Contributor Author

zooba commented Feb 15, 2019

My main concern is that I think this approach makes it the config file that will be modified by pip config set --global …

Indeed it is, and I don't like that, so I changed it to insert(0, sys.prefix) instead of .append().

@pfmoore
Copy link
Member

pfmoore commented Feb 15, 2019

Be careful here - we have both a local copy of appdirs and a vendored one, and I don't think they are consistently used. It's a mess that should be sorted at some point (probably by switching to just using the vendored one, which means patching appdirs will be something we'd want to avoid).

@zooba
Copy link
Contributor Author

zooba commented Feb 15, 2019

In my search of the current source tree, the vendored one is used once in the vendored pkg_resources, and not in pip's codebase itself. And the only function used in pip itself is one that doesn't exist in the vendored one at all, so I suspect we'd be better off removing the vendored one and keeping the fork.

@zooba zooba closed this Feb 15, 2019
@zooba zooba reopened this Feb 15, 2019
@zooba
Copy link
Contributor Author

zooba commented Feb 15, 2019

Rerunning CI, and hopefully the macOS build won't fail with an HTTP timeout this time.

@pfmoore
Copy link
Member

pfmoore commented Feb 15, 2019

the vendored one is used once in the vendored pkg_resources

Ah, thanks, it looks like things have changed since I last looked. Sorry for the noise.

@zooba
Copy link
Contributor Author

zooba commented Feb 15, 2019

So it seems macOS doesn't like files.pythonhosted.org today, for some reason. I'll requeue the build next time I take a look, assuming there's no further discussion or changes needed before then.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like this to be handled in a very different manner, by making changes to pip's existing configuration pipeline code. That'll put this new file in the same footing as all the others -- being able to use pip config on it.

Here's how I'd suggest doing this:

  • Add the configuration file path in pip._internal.locations
  • Add the relevant details to parts of pip._internal.configuration
    • kinds
    • Configuration (_load_config_files, _override_order, valid_load_only)
    • I think there might be a couple of other things to change here
  • Add an extra option to pip config to enable the ablity to manage this file via pip's CLI

src/pip/_internal/utils/appdirs.py Outdated Show resolved Hide resolved
@zooba
Copy link
Contributor Author

zooba commented Feb 16, 2019

So in terms of naming, I would consider this the "site config file", but that name is already used to mean global config files (and is already a list of files, which is why I added to that one). Suggestions?

@zooba
Copy link
Contributor Author

zooba commented Feb 16, 2019

Alternatively, make venv actually mean the current environment, regardless of whether it's a venv or not. Which basically means just removing the running_under_virtualenv() check.

@pradyunsg
Copy link
Member

pradyunsg commented Feb 17, 2019

As for the current approach, I've already stated that I'm okay with it, as well as suggested a renaming of venv in that case, modifying pip config in the appropriate manner to deprecate and rename functionality.

#5060 (comment)

@zooba
Copy link
Contributor Author

zooba commented Feb 19, 2019

suggested a renaming of venv

Did you suggest something to rename it to (which I can't find right now)? Or would you like to?

I personally like the name site, though that may be confusing in the codebase since all the functions for getting global paths are already called site. Otherwise, I like env (just drop the v). Do you have a preference?

(Obviously venv remains as an alias, and I can also add a bit more logic to make sure that it continues to fail if you're not in a virtual environment?)

@pradyunsg
Copy link
Member

pradyunsg commented Feb 20, 2019

@zooba I'd suggested ''changing the "venv" to "site" for pip config'' in the comment linked in my previous comment. I think we should, if we make this change (and do cleanups in the "locations" file too, for site vs global clarification, while we're doing that). That said, I do think we should first figure out if this is the change we want to make. :)

make venv actually mean the current environment, regardless of whether it's a venv or not. Which basically means just removing the running_under_virtualenv() check.

@dstufft and @pfmoore didn't seem very warm to this idea when I'd suggested this earlier, in the previously linked comment. I'm not sure how we'd address the concerns raised so I guess inputs from them would be most useful here.

@zooba
Copy link
Contributor Author

zooba commented Feb 20, 2019

Then I'll implement that change so it's concrete enough to argue about :)

@pfmoore
Copy link
Member

pfmoore commented Feb 20, 2019

My concern was about the fact that this was a request for special-case processing for conda. But as it's so simple to do, I don't really object to the change on a technical basis.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTM that there wasn't any existing test to ensure that pip config fails when given multiple "file-options".

Could you add that and change "if not file_options:" to verify the filtered length is 1?

(On mobile)

@zooba
Copy link
Contributor Author

zooba commented Feb 22, 2019

I added the test.

The if not file_options block already has a check for that - the order of handling there is "no options (use default), one option (use it), else raise". (Personally I prefer to put the error and early-return cases first so that you know you can stop reading there, but I chose not to reorder what was already here.)

@zooba
Copy link
Contributor Author

zooba commented Feb 26, 2019

This PR is just awaiting maintainer feedback/merge.

@pradyunsg pradyunsg requested a review from a team February 26, 2019 16:08
@zooba
Copy link
Contributor Author

zooba commented Mar 6, 2019

Ping @pypa/pip-committers (oh hey, I can annoy all of you at once ;) )

@pradyunsg pradyunsg changed the title Fixes #5060 Configuration files may now also be stored under sys.prefix Configuration files may now also be stored under sys.prefix Mar 6, 2019
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except for that one change regarding the deprecation warning. I'll merge once that's fixed. :)

Simplify selection of the best file option.
@pradyunsg pradyunsg merged commit 293c91e into pypa:master Mar 7, 2019
@pradyunsg
Copy link
Member

Thanks @zooba for working on this! :)

@zooba zooba deleted the issue-5060 branch March 7, 2019 15:29
@lock
Copy link

lock bot commented May 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to look for pip.conf in conda environment
4 participants